fix(terminal): non-Latin input, per-client PTY ownership, and reliable delivery - #181
Merged
Merged
Conversation
danyaberezun
requested review from
Olga Lavrichenko (OLavrik) and
Rinat S (rsolmano)
as code owners
August 6, 2026 18:24
danyaberezun
force-pushed
the
fix/terminal-non-latin-input-and-lifecycle
branch
from
August 7, 2026 12:25
630f204 to
3563416
Compare
The harness points HOME at an isolated dir so skill discovery never reads a
developer's real libraries. zsh finds no rc files there, concludes it is a
brand-new install, and blocks the terminal on its interactive
`zsh-newuser-install` wizard ("--- Type one of the keys in parentheses ---").
The wizard then swallows every keystroke a terminal spec sends, so
`terminals.spec.ts` failed on any machine whose $SHELL is zsh — reproduced on a
clean checkout of main, independent of any product change.
Seed empty .zshrc/.bashrc into the isolated home so an interactive shell starts
silently with a predictable prompt, whichever shell the developer runs, and
terminal specs stop depending on the host's dotfiles.
A user reported the terminal misbehaving after switching to a Russian layout. Three independent causes, none of them xterm's — its own chord and printable paths handle direct non-Latin layouts correctly. The PTY inherited no locale. A GUI-launched host (Finder/Dock, launchd, a container) gets none, which leaves bash/readline byte-oriented: one backspace deletes half of a two-byte character and the line desyncs from what the shell holds. Verified against a real PTY — "привет" reports 12 with no locale, 6 with one. `resolveShellEnv` now repairs the locale as well as PATH, treating the two as independent so the PATH short-circuit no longer skips it. Only LANG is set, and only when nothing at all is configured, so a user's per-category settings survive and an explicit LANG=C is not overruled. The rule is a pure exported `localeRepair()` — the same testable-seam role `pathLooksComplete` plays for PATH — and it lives in shellEnv rather than ptyEnv so the in-process agent's own bash gets the fix too. Two shortcuts matched `e.key`, the character a key produces, which is layout-dependent: on a Cyrillic layout R yields `к` and S yields `ы`, so both guards bailed before preventDefault() and the browser won instead — Ctrl+R reloaded the whole app, the exact behaviour useGlobalHotkeys exists to prevent, and Ctrl+S opened "Save page as" despite its comment promising to always swallow. Both now match `e.code`, which also agrees with the terminal one layer down (xterm resolves chords through keyCode, derived from the US layout). xterm measures the character cell once at construction and never again, unlike Monaco, which treats an early measurement as untrusted. Our code font ships as per-alphabet woff2 subsets, so the Cyrillic file lands later — non-Latin glyphs were sized for the fallback font and the PTY held the wrong cols/rows. Adds @xterm/addon-web-fonts with initialRelayout disabled, so we drive the re-measure and know when to re-fit and push the corrected size to the shell. Also drops four addon dependencies pinned but never imported (webgl, image, ligatures, serialize — taking five transitive deps with them) and records the resulting choice, xterm.js on the DOM renderer, as architecture.md Decision #11 with the triggers for re-evaluating it. Loading addon-webgl would be a regression, not an upgrade: the DOM renderer is a prerequisite for touch support and WebglAddon.dispose() leaks its WebGL2 context, which our per-worktree terminal churn would hit. Regression-pinned in e2e: a Cyrillic character-count check in terminals.spec.ts, and a CDP-driven layout test in history-search.spec.ts that dispatches trusted events whose `key` and `code` disagree (Playwright's press() always sends the matching pair, so it passed both before and after). The chord test was confirmed to fail with the fix reverted.
`TerminalsPanel` is mounted only inside the shell's `hasActiveWorkspace` branch, and selecting a project row clears the active workspace. So the deliberate "project home" gesture unmounted every terminal of *every* workspace, and each unmount closed its PTY — silently killing whatever was running in one (a dev server, a watch build) on a single click. The tabs reappeared afterwards backed by brand-new empty shells, so nothing looked broken; panels/SPEC.md even claimed re-selecting a workspace "restores its view", true for editor tabs and false for terminals. Invert the ownership: the PTY now outlives the component. An unmount that leaves its tab in place detaches the shell into a module-level registry keyed by clientId, and the next mount re-adopts it. Resuming costs no round trip — `terminal.data` is already keyed by PTY id, so re-subscribing to the same id reconnects the stream. Only a genuinely closed tab kills its PTY, and the store is the authority on which case it is: closing a tab removes it before React unmounts, so a real close is already absent while an incidental unmount still finds it. The process survives; the painted scrollback does not (a remount is a fresh xterm buffer). The store stays transport-free, so the reap decision lives in the component rather than in `closeTerminalTab`. Separately, `terminal.create` now carries the client's measured cols/rows and the PTY spawns at that size. It previously always started at 80x24 and relied on a follow-up resize, so the shell's first prompt was laid out for the wrong width and then reflowed. The params are optional, so an older client still works, and they are forwarded whole rather than rebuilt because under exactOptionalPropertyTypes an absent `cols` and an explicit `cols: undefined` are different types — only the former means "use the default". Regression-pinned in e2e/terminals.spec.ts: a shell variable set before visiting Project Home is still set after returning. Confirmed to fail with the fix reverted (the replacement shell echoes an empty value).
Terminals were the one host resource that is per-client, but nothing in the
system knew that. Four consequences, fixed together because they are the same
missing concept.
**Output reached every client.** The host published every PTY's bytes to a
single `terminal.data` topic that *every* socket subscribed to, leaving each
browser to discard the frames that weren't its own. Anything typed or printed in
any terminal of any workspace — tokens, keys, private paths — was delivered to
every connected client, which matters all the more once the host is reachable
from a phone. Frames are now addressed to the owning client, and write/resize/
close reject an id the caller doesn't own, answering exactly as for an id that
never existed so probing reveals nothing.
**Ownership is keyed to the page, not the socket.** The transport reconnects on
its own, so socket-keyed ownership would either stop streaming for good or kill
a running shell on any hiccup. A `?client=` id on the socket URL spans
reconnects but not reloads; an abandoned client's PTYs are reaped on a grace
timer instead of on close. The id is minted lazily and NOT via
`crypto.randomUUID`, which is secure-context-only: called at module scope it
threw during `main.tsx`'s import over plain http from a LAN IP or a Tailscale
MagicDNS name — a blank page with the ErrorBoundary never mounted.
**A dead shell looked alive.** `pty.onExit` discarded the exit code and the wire
had no exit event, so typing `exit` left a tab with a blinking cursor writing
every keystroke to a dead id. `terminal.exit` now announces it, is held and
retried if the owner is momentarily away, and `terminal.alive` lets a tab
re-attaching to a shell it detached earlier confirm it is still there — the exit
event is only heard by a *mounted* terminal, and detaching happens precisely
when none is.
**Output was one WS frame per PTY read.** `bun-pty` exposes no `pause()` and
starts reading at spawn, so a shell cannot be slowed down; reads are now batched
(`outputBatcher.ts`, timer-only so it is unit-tested directly), a batch the
owner can't take is kept and retried, and held output is capped — dropping the
oldest and flagging `truncated` so the client marks the gap. Resize is debounced
and no-op resizes are skipped, so dragging the divider no longer fires an ioctl
and SIGWINCH per layout frame.
Also: the transport rejects requests that were in flight when a socket died
(queued ones still flush on reconnect) and no longer replays event channels to
late subscribers — replaying an append-only byte stream repainted stale output.
Ctrl+letter and Escape are recovered while a CJK IME is active, working around
xterm 6.0.0 dropping them outright (keyCode 229 has no case in its chord table,
upstream #6065). One shared `onThemeSwap` replaces three hand-copied
MutationObservers; terminal derivations move to the store module.
PROTOCOL_VERSION 24 -> 25.
Every new test was mutation-verified rather than trusted: three of them passed
against the broken code on the first attempt and were rewritten. The isolation
test asserted rendered text, which the client's own id filter made meaningless,
so it now inspects the wire; the IME test asserted typed text, which the tty
echoes even while the shell is blocked; and the detached-exit test used plain
`kill`, which an interactive zsh ignores. That last rewrite is what exposed the
re-attach bug `terminal.alive` fixes.
Fixes a Playwright glob regression this change caused: `routeWebSocket("**/ws")`
compiles to an anchored `^(.*/)ws$`, so it silently stopped matching once the
URL gained a query string — in an @agent spec no commit gate runs.
Extracts the parts of the terminal path that were still ad hoc in the panel and the host, and fixes what they got wrong: - `terminalSend` maps Bun's real `ServerWebSocket.send` contract instead of guessing: `> 0` delivered, `-1` enqueued-under-backpressure (so it WAS accepted), `0` dropped and must be retried. The previous `!== -1` test had both edges backwards — it discarded a frame Bun had queued and reported a dropped one as sent. - `outputBatcher` returns that three-state result and latches "blocked", so a flooding shell stops hammering a socket that already refused. - `completionQueue` keeps a terminal's dying output ordered ahead of its `terminal.exit`, per client, and survives a reconnect — the exit could previously overtake the last thing the shell printed. - `ptySizeSync` separates desired / in-flight / acknowledged grids, so a resize that failed is retried instead of being remembered as applied. - `terminalPrebindBuffer` replaces the loose pre-bind array: bounded, and it handles a shell that exits before `terminal.create` even returns its id. - `requestReplayCache` gives a reconnecting page exactly-once semantics for requests it replays, keyed per client, never evicting an in-flight entry. Each new module is unit-tested directly, plus two e2e specs: a `terminal.create` whose response was lost with its socket runs once, and final output arrives before the exit. Comments elsewhere that described a dropped socket as a read failure are updated — a dropped socket is now recoverable.
The abandoned-client reaper cleared the whole replay namespace 60s after a socket went away, in-flight entries included — contradicting the cache's own stated invariant that "in-flight entries are never evicted: that is the interval in which executing a duplicate would be most damaging". The page identity outlives that window. `dialog.selectDirectory` holds its frame for 30 minutes (a human is looking at a folder picker), the transport replays every unresolved frame on reconnect under the same id, and reconnect backoff caps at 10s — so it retries forever. An offline stretch longer than the grace window (a closed laptop, a wifi handover) was therefore enough to forget a running handler and let the returning page start it again: a second native picker beside the one still on screen. Any long-timeout mutation could likewise execute twice. `clearClient` now reports whether the page is really gone, declining while anything is unresolved, and the reaper re-arms instead of forgetting. Retirement is all-or-nothing: an unresolved request is proof the page may return, and its settled siblings are just as replay-addressable to it (already bounded by count and serialized weight). The shells still die on the first pass — only the retirement waits. It ends when the last handler settles, on a reconnect, or at `stop()`. Both halves of the contract are mutation-verified: reverting `clearClient` to the unconditional delete fails the new test, and so does a version that answers `false` but clears anyway.
The replay cache bounded settled results by count and serialized weight and evicted the oldest to stay under it. Nothing in that decision consulted whether the page had actually received the result being thrown away. A successful `send` is not delivery: a socket that dies with a reply still in its buffer is indistinguishable from one that flushed it. So an evicted entry could be exactly the one the page still holds unresolved and replays on reconnect — and with the entry gone the host treats that replay as new work. A `terminal.create` whose response was lost would create a second PTY. The window is real: the transport keeps queueing requests while offline and sends them all at once on reconnect, so a burst can cross the bound before the replay that needs the old entry has been served. Two halves. The client now acknowledges every response it reads — `WsAck`, batched on a microtask, held across a disconnect and sent on the next socket after the replays. That receipt is the host's only evidence a reply was not lost, and it is what frees the retained copy; a client that reads its replies therefore keeps the cache near-empty and never approaches the bound at all. The bound still has to exist, for a peer that never acknowledges. It can no longer cause a duplicate: it reclaims the *result* and leaves the *id* behind as a tombstone, so a replay of a reclaimed request fails with a visible error instead of silently executing the work a second time. Exactly-once now holds regardless of how the peer behaves — only the answer can be lost, and only under pressure an acknowledging client never creates. Both halves are mutation-verified: reverting to unconditional eviction fails the new cache test, and dropping the tombstone turns that failure into the second execution the test names.
danyaberezun
force-pushed
the
fix/terminal-non-latin-input-and-lifecycle
branch
from
August 9, 2026 22:11
b55188a to
6f370f8
Compare
Two holes in the receipt scheme from the previous commit, both real.
A receipt is only as reliable as the socket carrying it. `flushAcks` dropped
each id once `send()` accepted it, which — exactly like the responses this
protocol exists to protect — only queues bytes. When an ack died in the buffer
nothing ever re-sent it: the page had already removed that request from
`pending`, so it was neither replayed nor acknowledged again, and the host held
the result until the page retired.
And tombstones were excluded from both limits, so those stranded entries, or a
v27 peer that simply withholds acks, grew the namespace without bound — the
backstop did not actually back anything.
Rather than confirm the confirmations, each reconnect now restates the truth.
`WsResume` (`{ resume: [ids] }`) is sent ahead of the replays and names every
id the page still considers unresolved; the host frees all other settled
results for that client. One lost receipt now costs one entry until the next
connect instead of forever, and receipts stay deliberately best-effort.
Memory is bounded at the other end. The namespace has a hard request-count and
serialized-byte cap, enforced on admission: a full namespace refuses *new* ids
with `RequestReplayOverflowError` (a normal `ok: false`) while still answering
every id it already holds. Refusing work that provably has not run is the one
bound that cannot cost exactly-once — which means nothing is ever evicted, and
the tombstone machinery the cap replaces is gone entirely.
In-flight entries remain exempt from all three release paths: a client cannot
have read a response that does not exist yet, and dropping a running handler is
the duplicate this cache exists to prevent.
Collaborator
Author
|
@jetbrains-air please re-review — both findings addressed in c69f874 (reconnect reconciliation for lost receipts; admission-control cap replacing the unbounded tombstones, so the cache now evicts nothing at all). Details in the thread reply. |
The byte cap was checked at admission, where it cannot work. A handler's output size is unknown until it finishes — `fs.readFile` returns a whole file, uncapped — and in-flight entries weigh nothing, so up to `maxRequestsPerClient` responses could all be admitted against an empty budget and settle arbitrarily far past it. Since nothing is ever evicted, they then stayed. The count was the only limit that was really holding. The two costs are now bounded separately, each where its size becomes known: entries on the way in (admission, unchanged), retained bytes on the way out of the handler. A result that would breach the budget is not retained — the entry stays as proof the work ran, so a replay fails with `RequestReplayUnretainedError` instead of executing it a second time. The normal path is untouched: the response was already sent, and the caller never sees this. Only a replay of an oversized response degrades, and it degrades to a visible error rather than a duplicate. Both limits remain unable to cost exactly-once — one refuses work that has not started, the other keeps the record of work that finished and drops only its answer.
Collaborator
Author
|
@jetbrains-air please re-review — the byte budget is now enforced in |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Started from one bug report — a user's terminal misbehaved after switching to a Russian keyboard layout — and
the investigation turned up several deeper problems in the terminal stack. Also includes research on whether
xterm.js is still the right choice (it is;
architecture.mdDecision #11 records why, plus the two triggersfor revisiting it).
The reported bug
Three independent causes, none of them xterm's:
one backspace deletes half of a two-byte character. Verified against a real PTY:
приветmeasures 12 with nolocale, 6 with one.
resolveShellEnvnow setsLANGwhen nothing is configured (onlyLANG, soper-category settings survive, and an explicit
LANG=Cis left alone).e.key, the character a key produces. On a Cyrillic layoutCtrl+Ryieldedк, sothe guard bailed and the browser reloaded the whole app — the exact thing that hook exists to prevent. Both
now match
e.code.Cyrillic file lands later and non-Latin glyphs were sized for the fallback font.
Bugs found along the way
workspace is selected, so "project home" unmounted every terminal and closed its PTY. Tabs came back looking
fine, backed by new empty shells. PTYs now outlive the component and are re-adopted.
terminal.datatopic that all sockets subscribed to,filtered client-side. Anything typed or printed anywhere reached every connected client. Frames are now
addressed to the owning client, and ops on an id you don't own behave like an id that doesn't exist.
pty.onExitdiscarded the exit code and the wire had no exit event, so typingexitleft a blinking cursor writing into nothing.crypto.randomUUIDat module scope is secure-context-only, sofrom a LAN IP or a Tailscale name it threw during import — blank page, ErrorBoundary never mounted. That is
the phone-over-Tailscale path the product is built for.
Notes for review
?client=), not a socket. The transport reconnects on its own, sosocket-keyed ownership would either stop streaming or kill a live shell on any hiccup. Abandoned clients are
reaped on a grace timer.
bun-ptyexposes nopause()and starts reading at spawn, so a shell genuinely cannot be slowed down.Output is batched, held and retried, and capped — dropping oldest and flagging
truncated. Worth an upstreamrequest.
PROTOCOL_VERSION24 → 25.first try and were rewritten: one asserted rendered text that the client's own id filter made meaningless,
one asserted typed text that the tty echoes anyway, and one used plain
kill, which interactive zsh ignores.Rewriting that last one is what exposed the re-attach bug
terminal.alivefixes.@agentspec:routeWebSocket("**/ws")compiles to an anchored^(.*/)ws$, so it silently stopped matching once the URL gained a query string.Verification
bun run e2e166 passed ·bun run test· typecheck · lint ·check:deps·check:seams·build:binary+smoke:binary. No suppressions added. Terminal specs went from 3 to 13.One thing to watch:
projects.spec.ts:81failed intermittently — 2 of 8 full runs on this branch,including 3 consecutive clean runs at the end;
mainwas clean 2 of 2. It is pure UI timing (focus, CSSopacity, right-click menu coordinates), touches nothing this branch changes, and passes in isolation on both
branches. Most likely a pre-existing load-sensitive flake, possibly nudged by the 11 new terminal specs
spawning real shells. Flagging it rather than hiding it; worth a look if CI trips on it.
Not done here
The mobile touch layer (quick-keys bar, soft-keyboard handling, touch scrolling) is deliberately out of
scope — it is the biggest remaining gap for a mobile-first product, needs real hardware, and Playwright cannot
simulate a soft keyboard. Also left alone: the stale terminal empty state, since which behaviour you want is a
product call.